-
-
Notifications
You must be signed in to change notification settings - Fork 359
Implemented Stable Marriage in Ruby #534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented Stable Marriage in Ruby #534
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! No dealbreakers, but a lot of things I think could be cleaned up. Nice usage of obscure standard functions, but I found a couple you missed :)
Will look into your suggestions next week. Thanks! |
Update stable_marriage.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took your suggestions into account. Pretty much straight forward changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really do not consider the display code you had self-explanatory at all. Thank you for changing it.
My complaint with shuffle!
wasn't its complexity (as you said, it's hardly unusual), just that it's meant for in-place operations and you're using it to chain operations. Thank you for changing that, too.
I am sorry, I totally missed this review. If @nic-hartley is happy, so am I! |
Code should be self explanatory